-
Notifications
You must be signed in to change notification settings - Fork 658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOCS: Align.py changes made #4182
Conversation
Linter Bot Results:Hi @MohitKumar020291! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## develop #4182 +/- ##
===========================================
- Coverage 93.61% 93.32% -0.30%
===========================================
Files 193 193
Lines 25170 25170
Branches 4059 4059
===========================================
- Hits 23562 23489 -73
- Misses 1092 1158 +66
- Partials 516 523 +7
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does your solution work?
Have you checked that your solution works? Show a screen shot when you run your code in a Python session.
Have you run the doc tests? According to #3925 you can run doc tests with the commands
cd package/doc/sphinx
make doctest
or alternatively follow #760 (comment) and invoke via pytest
cd package/MDAnalysis
pytest . --disable-pytest-warnings --doctest-modules
Does the test pass now? Show evidence: Show that you don't have the line
FAILED analysis/align.py::MDAnalysis.analysis.align.rotation_matrix
anymore.
New author
- Add yourself to the file AUTHORS.
- Send an email to the developer list introducing yourself (and mention this PR); if you've already done so, add a link to the email in the list archive in the comments of this PR.
The AUTHORS file is package/AUTHORS. The error that you encounter may be due to not having installed MDAnalysis. I suggest you follow the instructions in the user guide to create a development environment with conda https://userguide.mdanalysis.org/stable/contributing_code.html#creating-a-development-environment and then install a development version with If it still fails, describe step by step what you did. (Copy and paste code into the issue, that’s better than images; if only images are possible then you can directly drop them here, too) |
@orbeckst thanks for review. I tried multiple ways of creating universe but doctests failed. I am attaching the screenshots, please help to solve them. |
@MohitKumar020291 the second screenshot corresponds to the issue outlined by @orbeckst. In the first screenshot I think the issue is that you are using TPR and TRR within strings, while they should be passed directly. |
Thanks @RMeli for review. I tried passing TRR and TPR directly. But the results are not desirable. Screenshot attached. |
@RMeli and @orbeckst. I have made some latest change. I imported align as to use RELATED TO ABOVE IMAGE
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid skipping and make the doc test fully work (see inline comment).
Note I can quickly run the doc test with
cd package/MDAnalysis
pytest -v --disable-pytest-warnings --doctest-modules analysis/align.py
I suggest you remove the # doctest: +SKIP
, then run the above command. It will immediately tell you all you need to know to fix the problem.
Please also add yourself to AUTHORS and introduce yourself on the mailing list (see my previous review comments) — we won't merge a new contribution without the authorship/email information.
Great that you identified another problem in the same file. It would be great if all tests in align.py were to pass — please, feel free to also fix this in the same PR here! It looks as if it's just a missing closing parenthesis.
|
I had a quick look at align.py: there are a few more errors. The "missing parenthesis" is only the first one (and it's not really a missing parenthesis, just an incorrect line continuation). If you want to, you can quickly work through all of them and make this a very nice PR:
At the end there's an example that uses the clustalw program: just skip all of it with >>> seldict = align.fasta2select('sequences.aln') # doctest:+SKIP
>>> alignment = align.AlignTraj(trj, ref, filename='rmsfit.dcd', select=seldict) # doctest:+SKIP
>>> alignment.run() # doctest:+SKIP as this example is difficult to run under all circumstances (because clustalw is not always installed) and we don't have all input files at hand. |
I tried finding "how to add myself to changelog and author?", but did not found helpful results. So, I tried copying format in AUTHOR but I can't see 2023 in package/AUTHORS, so, I created my one(2023) which is obviously causing errors. Any solution to it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the one test that I mentioned in the comments.
The AUTHORS file can be found in the checked out sources as package/AUTHORS
. When you are in the top level directory that you got from git clone
you should be able to cd packages
and then edit the AUTHORS file.
I am trying to understand why it's difficult for you to find the AUTHORS file. How do you edit the files that you are changing? |
@orbeckst I can find the AUTHORS file but not 2023 it has only 2022. I was trying to change it through Apart from that I have resolved the latest changes. |
@MohitKumar020291 you might have an older version of the repository and the errors you see are conflicts? |
@RMeli I have latest version even then I will check it again. Yes, they were conflicts. |
THanks for introducing yourself on the developer list. The current AUTHORS file is on your branch: I can see it in the web interface https://github.com/MohitKumar020291/mdanalysis/blob/3365/package/AUTHORS. Maybe you have to EDIT: The AUTHORS addition is the only thing that's missing before we can approve the PR and squash-merge. |
@orbeckst I have successfully updated AUTHORS file. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding yourself. I looked at your solution again. ALthough it works, it can be written in a more succinct manner. In order to keep the example code as simple as possible, I'd like you to change your code. I made suggestions that you should be able to simply accept and include. Thanks!
Co-authored-by: Oliver Beckstein <[email protected]>
Co-authored-by: Oliver Beckstein <[email protected]>
@orbeckst changes done. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thank you for your contribution.
I'll just wait until the CI comes back all green and then squash-merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's now an issue with actually building the documentation, see https://github.com/MDAnalysis/mdanalysis/actions/runs/5439978586/jobs/9892463278?pr=4182#step:5:3019 . There's something wrong with the formatting. Please investigate. The build docs check must also pass.
It is possible that you need to indent the code blocks under ::
.
Literal block issue
skip
@orbeckst I tried finding issues multiple times with formatting but I did not get anything. |
I'd recommend that you
instead of having to wait for a while until the CI on GitHub runs. To build the docs locally have a look at our User Guide: Building docs. You will need to set up a development environment in which you can build docs. Follow User Guide: Create a virtual environment (I would use conda/mamba). Unfortunately, the docs are a bit outdated. MDAnalysis/UserGuide#260 contains some instructions to be added to an updated User Guide version. In short, try in your conda environment (with python>=3.8):
Then you should be able to build the docs as described, in short cd package
python setup.py build_sphinx -E Look at the output to see where files are generated. Read the output for errors. |
@orbeckst I think I have fixed the issue build docs, this is failing because of code coverage. Can you please rerun the checks? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🎉
The codecov thing is an annoying spurious failure and not related to your PR.
Well done! Thank you for your contribution! |
Fixes #3365
Changes made in this Pull Request: This PR is resolving suggestions by @orbeckst in #4023.
PR Checklist
📚 Documentation preview 📚: https://mdanalysis--4182.org.readthedocs.build/en/4182/